Fix APP-B2Q (3/3): give notification-preferences a distinct path param to avoid reportID collision - #94688
Conversation
Fixes APP-B2Q error #4: '[createDynamicRoute] Query param reportID exists in both base path and dynamic suffix'. mergeQueryStrings threw whenever a key appeared in both the base path (active route) and the dynamic suffix, even when both carried the same value. On HybridApp the restored active route can already hold ?reportID=R1, so re-emitting the same reportID from a dynamic route threw. Now it only throws when the values actually differ, and collapses identical duplicates into one param. Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com>
|
I analyzed the failing checks. No code fix is needed — every automated check that runs the code has passed (Jest tests across all 8 shards, Bun tests, The only failing check is To clear this check,
The remaining |
Steps to reproduce
The app crashes.
VideoScreen.Recording.2026-07-09.at.12.21.04.AM.movRoot causeCritical finding: the crash depends on which entry screen opens the shared
And even if the merge didn't throw, the result would still be wrong: when the navigation state is built, query params are spread over path/parent params, so the inherited value would silently win over the intended one: FixGive the screen a distinct path-param name ( 1. Turn the route into a path param in NOTIFICATION_PREFERENCES: {
- path: 'notification-preferences',
+ path: 'notification-preferences/:notificationReportID',
entryScreens: [SCREENS.REPORT_SETTINGS.DYNAMIC_ROOT, SCREENS.DYNAMIC_PROFILE],
- getRoute: (reportID: string) => getUrlWithParams('notification-preferences', {reportID}),
- queryParams: ['reportID'],
+ getRoute: (notificationReportID: string) => `notification-preferences/${notificationReportID}` as const,
},2. Update the param type in [SCREENS.REPORT_SETTINGS.DYNAMIC_NOTIFICATION_PREFERENCES]: {
- reportID: string;
+ notificationReportID: string;
};3. Resolve the ID from whichever param the screen provides in function WithReportOrNotFound(props: TProps) {
+ const params = props.route.params;
+ const reportID = 'notificationReportID' in params ? params.notificationReportID : params.reportID;
const [betas] = useOnyx(ONYXKEYS.BETAS);
- const [report] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${props.route.params.reportID}`);
+ const [report] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`);
const [policy] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY}${report?.policyID}`);
- const [reportMetadata] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_METADATA}${props.route.params.reportID}`);
- const [reportLoadingState] = useOnyx(`${ONYXKEYS.COLLECTION.RAM_ONLY_REPORT_LOADING_STATE}${props.route.params.reportID}`);
+ const [reportMetadata] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_METADATA}${reportID}`);
+ const [reportLoadingState] = useOnyx(`${ONYXKEYS.COLLECTION.RAM_ONLY_REPORT_LOADING_STATE}${reportID}`);
...
- const isReportIdInRoute = !!props.route.params.reportID?.length;
+ const isReportIdInRoute = !!reportID?.length;
...
- openReport({reportID: props.route.params.reportID, introSelected, betas});
- }, [shouldFetchReport, isReportLoaded, props.route.params.reportID]);
+ openReport({reportID, introSelected, betas});
+ }, [shouldFetchReport, isReportLoaded, reportID]);4. Pass the target report through - onPress={() => Navigation.navigate(createDynamicRoute(DYNAMIC_ROUTES.NOTIFICATION_PREFERENCES.path))}
+ onPress={() => {
+ if (!reportID) {
+ return;
+ }
+ Navigation.navigate(createDynamicRoute(DYNAMIC_ROUTES.NOTIFICATION_PREFERENCES.getRoute(reportID)));
+ }}Full code changes
NOTIFICATION_PREFERENCES: {
- path: 'notification-preferences',
+ // `reportID` is intentionally carried as a distinct path param (`notificationReportID`) rather than
+ // `reportID`, so it never collides with a `reportID` inherited from the surrounding report chain's
+ // query string. This keeps the inherited `?reportID=` intact for back navigation.
+ path: 'notification-preferences/:notificationReportID',
entryScreens: [SCREENS.REPORT_SETTINGS.DYNAMIC_ROOT, SCREENS.DYNAMIC_PROFILE],
- getRoute: (reportID: string) => getUrlWithParams('notification-preferences', {reportID}),
- queryParams: ['reportID'],
+ getRoute: (notificationReportID: string) => `notification-preferences/${notificationReportID}` as const,
},
[SCREENS.REPORT_SETTINGS.DYNAMIC_NOTIFICATION_PREFERENCES]: {
- reportID: string;
+ notificationReportID: string;
};
function WithReportOrNotFound(props: TProps) {
+ const params = props.route.params;
+ // Most screens carry the report ID under `reportID`. The notification-preferences screen instead
+ // owns its target report as a distinct path param (`notificationReportID`) so it never collides
+ // with a `reportID` inherited from the surrounding report chain in the URL.
+ const reportID = 'notificationReportID' in params ? params.notificationReportID : params.reportID;
const [betas] = useOnyx(ONYXKEYS.BETAS);
- const [report] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${props.route.params.reportID}`);
+ const [report] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`);
const [policy] = useOnyx(`${ONYXKEYS.COLLECTION.POLICY}${report?.policyID}`);
- const [reportMetadata] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_METADATA}${props.route.params.reportID}`);
- const [reportLoadingState] = useOnyx(`${ONYXKEYS.COLLECTION.RAM_ONLY_REPORT_LOADING_STATE}${props.route.params.reportID}`);
+ const [reportMetadata] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_METADATA}${reportID}`);
+ const [reportLoadingState] = useOnyx(`${ONYXKEYS.COLLECTION.RAM_ONLY_REPORT_LOADING_STATE}${reportID}`);
const [isLoadingReportData] = useOnyx(ONYXKEYS.IS_LOADING_REPORT_DATA);
const [introSelected] = useOnyx(ONYXKEYS.NVP_INTRO_SELECTED);
const isFocused = useIsFocused();
const contentShown = React.useRef(false);
- const isReportIdInRoute = !!props.route.params.reportID?.length;
+ const isReportIdInRoute = !!reportID?.length;
...
- openReport({reportID: props.route.params.reportID, introSelected, betas});
+ openReport({reportID, introSelected, betas});
// eslint-disable-next-line react-hooks/exhaustive-deps
- }, [shouldFetchReport, isReportLoaded, props.route.params.reportID]);
+ }, [shouldFetchReport, isReportLoaded, reportID]);
- onPress={() => Navigation.navigate(createDynamicRoute(DYNAMIC_ROUTES.NOTIFICATION_PREFERENCES.path))}
+ onPress={() => {
+ if (!reportID) {
+ return;
+ }
+ Navigation.navigate(createDynamicRoute(DYNAMIC_ROUTES.NOTIFICATION_PREFERENCES.getRoute(reportID)));
+ }} |
|
@MelvinBot Please implement this proposal #94688 (comment) instead of your proposal and update the OP |
Replaces the createDynamicRoute mergeQueryStrings change with ahmedGaber93's proposal: route the notification-preferences target report through a distinct `notificationReportID` path param so it can never collide with a `reportID` inherited from the surrounding report chain's query string. No changes to the core dynamic-route functions. Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com>
|
🤖 Done — implemented your proposal (comment) instead of the What changed (pushed to
Note: Verification:
Please fill in the human-only Tests/QA sections and add screenshots/videos before marking this Ready for review. |
|
🤖 The only failing check is PR Author Checklist / checklist — it fails because none of the 41 boxes in the PR description's checklist are ticked ( This isn't something I can fix by pushing code — the author checklist is a human sign-off, and I'm not permitted to tick those boxes (or fill the If any of the still-pending checks ( |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@MelvinBot Please update Tests
Offline testsSame as Tests step QA StepsSame as Tests step PR Author Checklistmark all checklist as checked |
|
@MelvinBot Please merge main branch into this |
|
🤖 Updated the PR description as requested:
|
…eQueryCollision Co-authored-by: ahmedGaber93 <ahmedGaber93@users.noreply.github.com>
|
🤖 Merged the latest Merge commit: |
|
@MelvinBot Mark this PR ready for review |
|
@ahmedGaber93 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
🤖 Marked this PR as ready for review. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9b3d42055
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // `reportID` is intentionally carried as a distinct path param (`notificationReportID`) rather than | ||
| // `reportID`, so it never collides with a `reportID` inherited from the surrounding report chain's | ||
| // query string. This keeps the inherited `?reportID=` intact for back navigation. | ||
| path: 'notification-preferences/:notificationReportID', |
There was a problem hiding this comment.
With this required path segment, URLs generated by the previous version such as .../notification-preferences?reportID=123 no longer match this dynamic route: the dynamic suffix matcher only sees the single notification-preferences segment, but the static suffix was removed and the new linking config requires :notificationReportID. On web, users can still land on those URLs from browser history, bookmarks, or a refresh across deploys, so this turns an existing notification-preferences deeplink into a fallback/not-found route; consider accepting the legacy query-param form while using the new path-param form for new navigations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I don't think legacy support for old URLs is necessary here, since this is a deeply nested route and isn't commonly used.
|
🤖 I looked into the failing check — nothing to fix in the code here. The only failing check is What's needed: the assigned reviewer just needs to fill out and submit the PR Reviewer Checklist — there's no code change for me to push. Once the checklist is completed, this check will go green. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-07-09.at.8.27.20.PM.movAndroid: mWeb ChromeScreen.Recording.2026-07-09.at.8.51.52.PM.moviOS: HybridAppScreen.Recording.2026-07-09.at.7.13.01.PM.moviOS: mWeb SafariScreen.Recording.2026-07-09.at.7.15.27.PM.movMacOS: Chrome / SafariScreen.Recording.2026-07-09.at.12.27.25.AM.mov |
ahmedGaber93
left a comment
There was a problem hiding this comment.
More explanation about the root cause and the proposed fix is available in this comment: #94688 (comment).
Alternatively, we could avoid this complexity by hiding Notification preferences on the profile page when it is opened from Details > Members. This behavior is not particularly important because changing the notification preference from that screen actually updates the member's DM conversation, not the currently opened chat. Users can still update the notification preference for the current chat from Chat > Settings > Notification preferences.
With this approach, the behavior would be:
- In any chat > Settings, users can still edit the notification preference for the current chat.
- In any chat > Details > Members > Member X, users cannot edit the notification preference for the DM conversation with that member.
|
I think this is ok to give it a go |
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.4.34-0 🚀
|
Help site review — no docs changes requiredI reviewed the changes in this PR against the help site articles under Why: This is an internal crash fix, not a user-facing behavior or feature change. It renames the notification-preferences route param ( Since the help site documents user-facing product behavior (features, settings, workflows) and nothing user-visible changed here, there is no corresponding article to update, so I did not create a draft PR.
@ahmedGaber93 if you believe a user-facing behavior did change here and a help article should be updated, let me know which flow and I'll draft the docs PR. |
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.4.34-14 🚀
|
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.4.34-14 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
This is 3 of 3 independent fixes for the JS throws bucketed under Sentry APP-B2Q (
#93845). It addresses error #4:[createDynamicRoute] Query param "reportID" exists in both base path and dynamic suffix.NOTIFICATION_PREFERENCESdeclaredreportIDas a query param. When the screen is opened from a member's Profile with.getRoute(report.reportID), the suffix contributes?reportID=<memberDM>while the base URL already carries a different?reportID=<reportInChain>inherited from the ancestor report chain.createDynamicRoutemerges the two query strings and throws on any key present on both sides — that uncaught error is the crash. (Opening the same screen from report settings used.pathwith no explicitreportID, so it inherited the same value and never conflicted.)The fix gives the screen a distinct path-param name (
notificationReportID) so it can never collide with an inheritedreportID, and it keeps the inherited?reportID=intact for back navigation. This needs no changes to the core dynamic-route functions (createDynamicRoute,getStateForDynamicRoute), so the original guard against genuinely contradictory query params is preserved for every other route.NOTIFICATION_PREFERENCESbecomes a path param and itsgetRoutereturnsnotification-preferences/<id>(noqueryParams).reportID→notificationReportID.withReportOrNotFoundresolves the id from whichever param the screen provides, using type-safeinnarrowing (no type assertions), so every other screen keeps readingreportIDunchanged.getRoute.Code references
ROUTES.tsNOTIFICATION_PREFERENCEStypes.ts:1820-1822withReportOrNotFound.tsx:75-79DynamicReportSettingsPage.tsx:74-79getRoutesignature still takes the target reportID):ProfilePage.tsx:304Fixed Issues
$ #93845
PROPOSAL: #94688 (comment)
Tests
Offline tests
Same as Tests steps.
QA Steps
Same as Tests steps.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari